Security: fix open redirect + timing-safe HMAC verify#2
Security: fix open redirect + timing-safe HMAC verify#2devin-ai-integration[bot] wants to merge 2 commits intomainfrom
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const out = new Uint8Array(hex.length / 2); | ||
| for (let i = 0; i < out.length; i++) { | ||
| const byte = parseInt(hex.slice(i * 2, i * 2 + 2), 16); | ||
| if (Number.isNaN(byte)) return null; |
There was a problem hiding this comment.
WARNING: parseInt silently ignores non-hex trailing characters, so Number.isNaN alone does not fully validate the input.
In JavaScript, parseInt('0g', 16) returns 0 (not NaN), and parseInt('ag', 16) returns 10. This means a hex pair like '0g' passes the isNaN check and is silently coerced to 0x00 instead of being rejected. While this is unlikely to be exploitable in practice (the HMAC comparison would simply fail with an incorrect byte), it is a correctness bug — fromHex may accept malformed hex strings without returning null.
A safer check is to validate the full hex string with a regex before parsing:
if (!/^[0-9a-fA-F]+$/.test(hex)) return null;Or validate each pair with /^[0-9a-fA-F]{2}$/ before calling parseInt.
There was a problem hiding this comment.
Fixed in aa8bd44 — added a full-string regex validation (/^[0-9a-fA-F]+$/) plus an empty-string reject before the parseInt loop, so malformed pairs like "0g" return null rather than coercing to 0x00.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Previously Flagged — Now Resolved
Files Reviewed (2 files)
Reviewed by claude-4.6-sonnet-20260217 · 178,251 tokens |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1 participant
Summary
Addresses the two Devin Review flags from #1:
Open redirect in
/login?next=…— thenextform/query value was passed directly intoredirect(), so/login?next=https://evil.comwould bounce an authenticated user off-site. Now routed through asafeNext()helper that forcesnextto be a same-origin relative path (/…) and strips protocol-relative (//evil.com) and backslash-prefixed tricks. The hidden form input also uses the sanitized value so it can't be replayed.Timing-unsafe HMAC comparison in
lib/session.ts—expected === signedwas replaced withcrypto.subtle.verify("HMAC", key, sig, data), which does constant-time comparison under the hood. Added a smallfromHexhelper to decode the signature bytes. Still fully Edge-runtime safe (nonode:crypto).Build + lint pass locally.
Review & Testing Checklist for Human
https://kilo-control-8955.d.kiloapps.io/login?next=https://example.com→ sign in → should land on/, notexample.com./login?next=//evil.com→ should also collapse to/./login?next=/profiles→ should still land on/profilesafter login (normal case still works).kilo_control_sessioncookie value (flip a hex char in the signature) → reload → should bounce back to/login(verify still rejects invalid sigs).Notes
Same-branch deploy target is
https://kilo-control-8955.d.kiloapps.io. A fresh Kilo Deploy build should pick upmainonce this merges; the existing running container keeps serving in the meantime.Link to Devin session: https://app.devin.ai/sessions/a0624ee5b9a847728a5ed20d9d0481ab
Requested by: @itsablabla